Skip to content

Conversation

@jfreden
Copy link
Contributor

@jfreden jfreden commented Sep 9, 2025

The pattern to load secure settings for later use during secure settings reload has been repeated across the code base and is now needed by #134137. This PR extracts the functionality to a InMemoryClonedSecureSettings utility class.

@jfreden jfreden marked this pull request as ready for review September 9, 2025 09:07
@jfreden jfreden requested a review from a team as a code owner September 9, 2025 09:07
@jfreden jfreden added :Security/Security Security issues without another label >enhancement labels Sep 9, 2025
@jfreden jfreden requested a review from tvernum September 9, 2025 09:07
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Sep 9, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @jfreden, I've created a changelog YAML for you.

Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho if turning this into a general util, this requires a bit more care in a few places. See the comments below.

I'm just wondering, are there no security concerns around providing such a simple util to work around the purpose of secure settings?

@jfreden
Copy link
Contributor Author

jfreden commented Sep 11, 2025

I'm just wondering, are there no security concerns around providing such a simple util to work around the purpose of secure settings?

I'm suspecting that's why this has not been added before and that was also my hesitation. This utility class works against the current intention of the KeyStoreWrapper - that the settings are only available during the reload to refresh some state and then dropped.

The security implication is that the secure settings that are "always loaded" will be in memory and visible in a heap dump. The secure settings approach we're taking with the KeyStoreWrapper isn't immune to that, even if the settings are just loaded for a while they can be extracted and stored in some state past the reload.

I think ideally the settings would be extracted and stored in an instance variable of the class that uses it. The problem this PR solves is that the actual Settings object is needed at some later point by the code we're using and having some intermediate representation of the settings becomes hacky.

@tvernum
Copy link
Contributor

tvernum commented Sep 11, 2025

In practice, we need to retain some secure settings in memory.

They're used in places that are dynamic - typically because the secure setting is paired with some other dynamic cluster settings - and we can't clear them from memory after the first use.

We definitely don't want to retain all secure settings in memory, but making it hard to do the thing we have to do, doesn't help anyone. It just forces duplication of the same work-arounds, often with poorer security than we would have gotten if we'd just implemented something that filled the need.

I think we can come up with a better name that LoadedSecureSettings, one that reflect the compromise we've having to make, but I'm not sold on an argument that we should keep it out of core so that we end up with multiple reimplementations of the same thing.

@jfreden
Copy link
Contributor Author

jfreden commented Sep 11, 2025

Thanks for the feedback! I've changed the name to ClonedSecureSettings and added some more javadoc.

@jfreden jfreden changed the title Add LoadedSecureSettings for keeping temporary secure settings loaded Add ClonedSecureSettings for keeping temporary secure settings loaded Sep 11, 2025
@jfreden jfreden requested a review from mosche September 11, 2025 07:26
@mosche
Copy link
Contributor

mosche commented Sep 11, 2025

I think we can come up with a better name that LoadedSecureSettings, one that reflect the compromise we've having to make

How about InMemoryClonedSecureSettings or just InMemorySecureSettings to highlight that compromise?

but I'm not sold on an argument that we should keep it out of core so that we end up with multiple reimplementations of the same thing.

keeping it out of core was never my intention, if we highlight the compromise and make devs well aware (as done in the Javadocs now) I'm totally fine with this

Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm provided tests pass, just a few optional suggestions

@jfreden jfreden changed the title Add ClonedSecureSettings for keeping temporary secure settings loaded Add InMemoryClonedSecureSettings for keeping temporary secure settings loaded Sep 11, 2025
@jfreden jfreden added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 11, 2025
@elasticsearchmachine elasticsearchmachine merged commit fc358eb into elastic:main Sep 11, 2025
40 checks passed
@jfreden jfreden deleted the add_secure_settings_cloner branch September 11, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants